Use relative path for vm state directory #90
Conversation
dmcgowan
force-pushed
the
fix-vm-socket-path
branch
from
597497f to
e78ebbc
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes the vm state directory from being bundle-relative to current-working-directory-relative to prevent socket path length issues on macOS where path resolution can differ (e.g., /var vs /private/var).
Changes:
- Modified vm state directory creation to use
filepath.Abs("vm")instead offilepath.Join(r.Bundle, "vm") - Added error handling for the absolute path resolution
- Added a comment explaining the rationale for the change
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/shim/task/service.go
Outdated
| if err != nil { | ||
| return nil, errgrpc.ToGRPCf(err, "failed to resolve vm state directory") | ||
| } | ||
| if err := os.Mkdir(vmState, 0700); err != nil { |
There was a problem hiding this comment.
The use of os.Mkdir will fail if the directory already exists. Since multiple containers can share the same VM instance (as evidenced by the containers map and the vmInstance guard), and the VM state directory is now shared across all containers in the same working directory, the second Create call will fail when trying to create the directory again. Consider using os.MkdirAll or checking for os.IsExist error to allow the directory to already exist.
| if err := os.Mkdir(vmState, 0700); err != nil { | |
| if err := os.MkdirAll(vmState, 0700); err != nil { |
internal/shim/task/service.go
Outdated
| // vmState directory should be relative path to current working directory, not specific to a single bundle | ||
| vmState, err := filepath.Abs("vm") | ||
| if err != nil { | ||
| return nil, errgrpc.ToGRPCf(err, "failed to resolve vm state directory") |
There was a problem hiding this comment.
The error message states "failed to resolve vm state directory" but filepath.Abs doesn't resolve in the sense of following symlinks or checking if the path exists - it just converts a relative path to an absolute one. Consider changing the message to "failed to get absolute path for vm state directory" to more accurately describe what could go wrong.
| return nil, errgrpc.ToGRPCf(err, "failed to resolve vm state directory") | |
| return nil, errgrpc.ToGRPCf(err, "failed to get absolute path for vm state directory") |
internal/shim/task/service.go
Outdated
| b.AddExtraFile(nwcfg.Filename, nwJSON) | ||
|
|
||
| vmState := filepath.Join(r.Bundle, "vm") | ||
| // vmState directory should be relative path to current working directory, not specific to a single bundle |
There was a problem hiding this comment.
The comment states that vmState should be a "relative path to current working directory" but the code uses filepath.Abs which returns an absolute path, not a relative one. The comment should be updated to accurately reflect that the vmState is now an absolute path based on the current working directory (e.g., "vmState directory should be in the current working directory, not specific to a single bundle").
| // vmState directory should be relative path to current working directory, not specific to a single bundle | |
| // vmState directory is an absolute path under the current working directory, not specific to a single bundle |
Prevents socket paths becoming too long when the resolved current working directory differs from the given bundle path, for example current working directory resolving to `/private/var/...` but the bundle directory reported as `/var/...`. The vm state directory does not need to be associated with a specific bundle and the working directory assigned to the shim is sufficient as the parent for the vm state directory. Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
fix-vm-socket-path
branch
from
e78ebbc to
e1eaba4
Compare
2 participants
Prevents socket paths becoming too long when the resolved current working directory differs from the given bundle path, for example current working directory resolving to
/private/var/...but the bundle directory reported as/var/.... The vm state directory does not need to be associated with a specific bundle and the working directory assigned to the shim is sufficient as the parent for the vm state directory.